Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maybe: add S.fromMaybe_ #316

Merged
merged 1 commit into from
Dec 30, 2016
Merged

maybe: add S.fromMaybe_ #316

merged 1 commit into from
Dec 30, 2016

Conversation

davidchambers
Copy link
Member

Closes #296

@davidchambers davidchambers mentioned this pull request Dec 29, 2016
33 tasks
//. > global.fib = function fib(n) {
//. . return n <= 1 ? n : fib(n - 2) + fib(n - 1);
//. . }
//. fib
Copy link
Member

@Avaq Avaq Dec 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to read this three times before understanding that you're just using it as an example of a computationally expensive function. The fact that you explicitly state the return value of the function definition, and that you're explicitly attaching it to the global scope, makes no sense without knowledge of doctest. It seemed to me at first as if you're trying to demonstrate something here, whereas in fact you're really just working around the semantics of your linter.

I think it might be better to use something native to JavaScript (or in fact exposed by Sanctuary) to demonstrate. Maybe even something like /*perform expensive calculation*/ 1 + 1. Or if you really feel fib demonstrates it best, it would've helped me to see a comment above fibs definition explaining why it's being defined there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I see you've already taken the same approach for documenting maybe_. I suppose multiple instances justify the learning curve.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps if you wouldn't have to assert the return value of the function definition, it would read more easily, and it wouldn't look like a "stand-alone" example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for highlighting this, Aldwin. We are indeed working around a limitation of the tool. I just opened davidchambers/doctest#73. At some point I will address that issue, publish a new version of doctest, and update these fib definitions. This needn't all happen before this pull request is merged, though. :)

//. . return n <= 1 ? n : fib(n - 2) + fib(n - 1);
//. . }
//. fib
//. > function fib(n) { return n <= 1 ? n : fib(n - 2) + fib(n - 1); }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you go, Aldwin. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\o/

@davidchambers
Copy link
Member Author

$ npm install [email protected]

$ node
> const S = require('sanctuary')
undefined
> S.fromMaybe_
fromMaybe_ :: (() -> a) -> Maybe a -> a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants